Add HttpFilterIntegrationTestBase by oschaaf · Pull Request #517 · envoyproxy/nighthawk

oschaaf · 2020-09-09T17:04:47Z

Prelude to #512, which includes it and provides a means to
see the end goal.

Extracts shared needs between tested extensions.
Sanitized api, better naming.
Doc comments.
Support for testing POST request methods and entity bodies
to check the alternative flow that will trigger in extensions
when using that.

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

- Extracts shared needs between tested extensions. - Sanitized api, better naming. - Doc comments. - Support for testing POST request methods and entity bodies to check the alternative flow that will trigger in extensions when using that. Prelude to envoyproxy#512, which includes it and provides a means to see the end goal. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90 · 2020-09-09T17:42:12Z

@eric846 please review and assign to me when done.

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

eric846 · 2020-09-11T19:49:01Z

test/server/http_filter_integration_test_base.h

+   * up a fake upstream to supply the response.
+   *
+   * @param request_level_config Configuration to be delivered by request header. For example
+   * "{{response_body_size:1024}".


This looks like a typo.

eric846 · 2020-09-11T19:49:12Z

test/server/http_filter_integration_test_base.h

+   * extension under test should supply the response.
+   *
+   * @param request_level_config Configuration to be delivered by request header. For example
+   * "{{response_body_size:1024}".


eric846 · 2020-09-11T19:52:12Z

test/server/http_filter_integration_test_base.cc

+}
+
+Envoy::IntegrationStreamDecoderPtr
+HttpFilterIntegrationTestBase::getResponse(absl::string_view request_level_config,


Could we replace this getResponse overload with a helper that translates a string into an Envoy::Http::TestRequestHeaderMapImpl and use the helper inline?

eric846 · 2020-09-11T19:54:17Z

test/server/http_filter_integration_test_base.h

+   * @brief Fetches a response with request-level configuration set in the request header.
+   *
+   * @param request_level_config Configuration to be delivered by request header. For example
+   * "{{response_body_size:1024}".


eric846 · 2020-09-11T19:57:40Z

test/server/http_filter_integration_test_base.h

+  getResponse(const Envoy::Http::TestRequestHeaderMapImpl& request_headers,
+              bool setup_for_upstream_request);
+
+  const Envoy::Http::TestRequestHeaderMapImpl request_headers_;


What about default_request_headers_?

eric846 · 2020-09-11T20:00:08Z

test/server/http_filter_integration_test_base.h

+   * be used to inspect the response.
+   */
+  Envoy::IntegrationStreamDecoderPtr getResponse(absl::string_view request_level_config,
+                                                 bool setup_for_upstream_request);


Could we reduce the number of public methods by half and eliminate the need to figure out the meaning of true or false in the implementation by creating a mode enum with values kUpstream and kExtension? Or if it's more convenient for the test authors to have the separate public methods, a private enum could still make the impl easier to read.

eric846 · 2020-09-11T20:05:02Z

test/server/http_filter_integration_test_base.cc

+Envoy::IntegrationStreamDecoderPtr
+HttpFilterIntegrationTestBase::getResponse(absl::string_view request_level_config,
+                                           bool setup_for_upstream_request) {
+  const Envoy::Http::LowerCaseString key("x-nighthawk-test-server-config");


It would be good to mention in all doc comments that request_level_config is sent as the x-nighthawk-test-server-config header -- I didn't realize how it worked.

eric846 · 2020-09-11T20:14:26Z

test/server/http_filter_integration_test_base.h

+   * @return Envoy::IntegrationStreamDecoderPtr Pointer to the integration stream decoder, which can
+   * be used to inspect the response.
+   */
+  Envoy::IntegrationStreamDecoderPtr getResponseFromUpstream();


Are we missing getResponseFromExtension() with no args, or does that one not make sense?

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-09-12T00:00:42Z

Thanks for the review @eric846. Following some of your suggestions, I refactored this some more to further simplify in
8d30820. I think it captures most of your comments modulo the one about default_request_headers_, which no longer makes sense as it is now private and used in a stateful manner. If you want, it's possible to take a peek at an example that consumes the state as it is now proposed here over here.

eric846 · 2020-09-12T03:31:54Z

LGTM, thanks!

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-09-14T08:29:38Z

@eric846 Sorry -- added a small new helper here in 7eeb7e9 which I turned out to need, PTAL.

Split out from envoyproxy#512 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> ------ Prerequisite: - [ ] Land envoyproxy#517

dubious90 · 2020-09-14T18:57:44Z

test/server/http_filter_integration_test_base.h

+   *
+   * @param config configuration to pass to Envoy::HttpIntegrationTest::config_helper_.addFilter.
+   */
+  void setup(const std::string& config);


I think having a setup and a SetUp is confusing, especially since it's not really consistent with our capitalization scheme. Can we rename this to something like initializeConfig?

dubious90 · 2020-09-14T18:59:09Z

test/server/http_filter_integration_test_base.cc

+    : HttpIntegrationTest(Envoy::Http::CodecClient::Type::HTTP1, ip_version),
+      request_headers_({{":method", "GET"}, {":path", "/"}, {":authority", "host"}}) {}
+
+void HttpFilterIntegrationTestBase::setup(const std::string& config) {


Just to check - this can't be an absl::string_view?

Done -- this was slightly more convenient in the implementation as we need to copy, but I agree consistency outweighs that. Thanks for catching

dubious90 · 2020-09-14T20:53:58Z

test/server/http_filter_integration_test_base.cc

+  const bool is_post = request_headers_.Method()->value().getStringView() ==
+                       Envoy::Http::Headers::get().MethodValues.Post;
+  const uint64_t request_body_size = is_post ? 1024 : 0;
+  if (expected_origin == ResponseOrigin::UPSTREAM) {


I'm having trouble understanding this function's full intent. There are a lot of conditionals that seem to make the behavior very different. Is this actually common behavior across filters?

Maybe some comments in this section about why things are happening would be helpful.

I added some comments in ca17f41. Does that provide the type of context you'd consider helpful when reading this code?
Basically, this support call is an extract of the what we need to cover the stream codec code for all our extensions. We'll later see that this allows us deprecate much more complex code (which was the way to do it a while back), as wel as deduplicate some similar code.

Additionally, for some more concrete context, #533 is the direct follow up to this (currently in draft, as it depends on this PR, and will lean up once we merge this one and merge the result into that). It brings in a new test file which is using this extensively.
That test file adds tests for ensuring basic behavioural guarantees across all of our extensions.

The behaviours we are looking for involve certain configuration (error) handling, as well as handling default/empty configuration, both static from disk as well as dynamically delivered via response headers. These tests are parameterised over GET and POST requests.

Once we land that, we can in turn start purging / enhancing the other test files, and make them more about just testing the functionality that the extension brings in. Where applicable, it will also use the functionality we add in this PR, and we can purge their own variant(s).

(NOTE: the changes from the last paragraph above, are already staged over at #512)

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-09-14T21:49:37Z

@dubious90 feedback address in ca17f41, thanks for the review!

dubious90

Thank you for adding all of the comments. I understand what is happening here a lot more now. Have some suggestions on naming and documentation, but as always, open to pushback.

dubious90 · 2020-09-16T16:12:39Z

test/server/http_filter_integration_test_base.h

+namespace Nighthawk {
+
+/**
+ * Base class with shared functionality for testing Nighthawk test server http filter extensions.


Can we add two notes/clarifications here: 1. This class is stateful. 2. Unless I'm mistaken I don't think this class is threadsafe

dubious90 · 2020-09-16T16:23:50Z

test/server/http_filter_integration_test_base.h

+  HttpFilterIntegrationTestBase(Envoy::Network::Address::IpVersion ip_version);
+
+  /**
+   * We don't override SetUp(): tests using this fixture must call setup() instead.


nit: reference to setup() here is no longer accurate

dubious90 · 2020-09-16T16:30:02Z

test/server/http_filter_integration_test_base.cc

+  // This is useful, because emitting an entity body will hit distinct code in extensions. Hence we
+  // facilitate that.
+  const uint64_t request_body_size = is_post ? 1024 : 0;
+  // An extension can either act as an origin and synthesize a reply, or delegate that


Looking through again, these comments are very helpful. Thank you for adding them

dubious90 · 2020-09-16T16:36:02Z

test/server/http_filter_integration_test_base.h

+                        absl::string_view header_value);
+
+  /**
+   * Fetch a response. The request headers default to a minimal GET, but this may be changed


Can we rephrase this comment a little (correct anything here that is inaccurate):
Fetch a response, according to the options specified by the class methods. By default, simulates a GET request with minimal headers.

dubious90 · 2020-09-16T16:37:41Z

test/server/http_filter_integration_test_base.h

+   * We don't override SetUp(): tests using this fixture must call setup() instead.
+   * This is to avoid imposing the need to create a fixture per filter configuration.
+   *
+   * @param config configuration to pass to Envoy::HttpIntegrationTest::config_helper_.addFilter.


is this configuration for the filter being tested? If so, can we clarify "filter configuration"

dubious90 · 2020-09-16T16:38:54Z

test/server/http_filter_integration_test_base.h

+  /**
+   * Indicate the expected response origin.
+   */
+  enum class ResponseOrigin { UPSTREAM, EXTENSION };


Can we add a one line summary here for each type of ResponseOrigin? This may also allow you to remove a comment elsewhere, if it's duplication at this point.

dubious90 · 2020-09-16T16:44:48Z

test/server/http_filter_integration_test_base.h

+   * @param request_level_config Configuration to be delivered by request-header in future calls to
+   * getResponse(). For example: "{response_body_size:1024}".
+   */
+  void updateRequestLevelConfiguration(absl::string_view request_level_config);


nit/optional: How do you feel about changing this to setRequestLevelConfiguration? I think it'd then be more consistent with setRequestHeader

dubious90 · 2020-09-16T16:46:44Z

test/server/http_filter_integration_test_base.h

+   * Switch getResponse() to use the POST request method with an entity body.
+   * Doing so will make tests hit a different code paths in extensions.
+   */
+  void switchToPostWithEntityBody();


Just a thought, curious what you think - it might be more self-documenting here, if we take in a parameter for the post body size. Or even then, rename this to setPostBodySize, with an indication that unless this is called, the requests into the filter will be GET requests.

I would be more up for doing this, but rather do it once a functional need for it arises (which is very possible). Right now, in the context of this PR and the changes coming after this, I'm on a mission to minimize and contain complexity that has organically grown while we added the test server and the two new extensions. Part of effort is making everything go through this hopefully well defined but minimal functionality. It should be trivial to refactor this later on according to what you suggest, once the need arises in new tests?

That makes sense to me. I'm happy to push it through as is, and if it comes up, do the refactor as you suggest.

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-09-16T20:34:17Z

@dubious90 feedback addressed in dubious90 modulo one thing, I will reply to that specific comment to explain.

dubious90

LGTM. Thank you for addressing my concerns.

oschaaf added the waiting-for-review A PR waiting for a review. label Sep 9, 2020

oschaaf mentioned this pull request Sep 9, 2020

Test-server extension: use shared configuration base and test facilities #512

Merged

9 tasks

dubious90 requested a review from eric846 September 9, 2020 17:42

Merge remote-tracking branch 'upstream/master' into extension-refacto…

0c6f182

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

eric846 requested changes Sep 11, 2020

View reviewed changes

eric846 added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 11, 2020

oschaaf added 2 commits September 12, 2020 01:48

Review feedback

8d30820

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into extension-refacto…

4827909

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 12, 2020

oschaaf added 2 commits September 14, 2020 09:37

Merge remote-tracking branch 'upstream/master' into extension-refacto…

5f18691

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add setRequestHeader

7eeb7e9

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Sep 14, 2020

Add generic c++ integration test for uniform extension behavior. #533

Merged

1 task

dubious90 reviewed Sep 14, 2020

View reviewed changes

dubious90 added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 14, 2020

oschaaf added 2 commits September 14, 2020 23:36

Merge remote-tracking branch 'upstream/master' into extension-refacto…

e4a50bd

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

ca17f41

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 14, 2020

dubious90 self-assigned this Sep 15, 2020

dubious90 suggested changes Sep 16, 2020

View reviewed changes

oschaaf added 2 commits September 16, 2020 22:29

Merge remote-tracking branch 'upstream/master' into extension-refacto…

139cfff

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

2aff84a

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90 approved these changes Sep 16, 2020

View reviewed changes

dubious90 merged commit 5876039 into envoyproxy:master Sep 16, 2020

Conversation

oschaaf commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dubious90 commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Sep 12, 2020

Uh oh!

eric846 commented Sep 12, 2020

Uh oh!

oschaaf commented Sep 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Sep 14, 2020

Uh oh!

dubious90 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Sep 16, 2020

Uh oh!

dubious90 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

oschaaf commented Sep 9, 2020 •

edited

Loading

oschaaf Sep 15, 2020 •

edited

Loading